kvs: handle non-JSON values in flux kvs get, put, dir #1257

garlick · 2017-10-25T20:15:00Z

This PR updates the flux kvs command to handle non-JSON values. These deficiencies were discussed in #1158 and #1159.

A summary of the changes proposed here are:

flux kvs dir displays non-JSON values
flux kvs dir truncates output to fit terminal width, controlled with --width=COLS
flux kvs put and flux kvs get now accept the --raw, --json, and --treeobj options to allow greater control over how values are stored and interpreted.

As a reminder the KVS just stores raw values now, imposing no structure. We have convenience functions for get/put of strings (NULL terminated), and JSON (encoded as strings). The get/put subcommands now default to storing strings, as opposed to JSON-encoded strings. The --json option was added to emulate the past behavior. The --raw options was added to allow non-NULL terminated byte sequences to be stored and retrieved. The --treeobj option was added so that snapshots could be manipulated and metadata could be examined conveniently.

Man pages updated and sharness tests added.

coveralls · 2017-10-25T20:41:46Z

Coverage increased (+0.01%) to 78.505% when pulling 8135869 on garlick:kvs_cmd_update into 2694116 on flux-framework:master.

chu11 · 2017-10-25T21:50:34Z

just beginning to review, but a thought that comes to mind, are many options in t/kvs/basic axe-able? and tests adjusted accordingly? copy-fromkvs, copy-tokvs, get-treeobj, put-treeobj, spring to mind. Could be cleanup via another issue of course and not necessarily this PR.

garlick · 2017-10-25T21:53:30Z

Hmm, it does seem like get-treeobj and put-treeobj are obviously the same as get/put with the --treeobj option. I'll see if I can do some of that cleanup here.

chu11 · 2017-10-25T23:04:32Z

src/cmd/flux-kvs.c

@@ -56,6 +56,16 @@ static void dump_kvs_dir (const flux_kvsdir_t *dir, bool Ropt, bool dopt);



typo in commit comment, "strong" -> "string"

hmm, my review comment appears to be attached to file and not a commit. This is in commit f93f776

codecov-io · 2017-10-26T00:09:49Z

Codecov Report

Merging #1257 into master will increase coverage by 0.06%.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##           master    #1257      +/-   ##
==========================================
+ Coverage   77.86%   77.92%   +0.06%     
==========================================
  Files         154      154              
  Lines       28934    29027      +93     
==========================================
+ Hits        22530    22620      +90     
- Misses       6404     6407       +3

Impacted Files	Coverage Δ
src/cmd/flux-kvs.c	`80.06% <83.87%> (+0.28%)`	⬆️
src/common/libutil/blobref.c	`97.22% <0%> (-1.39%)`	⬇️
src/common/libflux/message.c	`81.13% <0%> (-0.59%)`	⬇️
src/common/libkvs/kvs_txn.c	`74.01% <0%> (-0.57%)`	⬇️
src/common/libutil/dirwalk.c	`94.28% <0%> (ø)`	⬆️
src/bindings/lua/flux-lua.c	`81.16% <0%> (+0.08%)`	⬆️
src/common/libflux/handle.c	`83.91% <0%> (+0.24%)`	⬆️
src/common/libkvs/kvs.c	`64.28% <0%> (+0.49%)`	⬆️
src/modules/kvs/kvs.c	`62.88% <0%> (+0.51%)`	⬆️
src/broker/modservice.c	`81.55% <0%> (+0.97%)`	⬆️
... and 2 more

garlick · 2017-10-26T00:16:28Z

Just rebased on current master, fixed commit comment, and added some commits that move functionality out of t/kvs/basic that now seem appropriate in flux-kvs:

get [--at ref]        # replaces t/kvs/basic getat
dir [--at ref]        # replaces t/kvs/basic dirat
readlink [--at ref]   # replaces t/kvs/basic readlinkat
put --raw key=-       # (read from stdin), replaces t/kvs/basic copy-tokvs
put [--no-merge]      # replaces t/kvs/basic put-no-merge

Good suggestion @chu11. All that's left in the test command is the type checking stuff.

chu11 · 2017-10-26T00:16:43Z

t/t1000-kvs.t

+	flux kvs put $DIR.valref=$(seq -s 1 100) &&
+	flux kvs mkdir $DIR.dirref &&
+	flux kvs link foo $DIR.symlink &&
+	flux kvs get --treeobj $DIR.val &&


perhaps | grep "dir" and equivalents should be added to make sure the right treeobj is being returned

Sure, that's a good improvement.

chu11 · 2017-10-26T00:21:36Z

t/t1000-kvs.t

+test_expect_success 'kvs: empty string value does not cause dir to fail' '
+	flux kvs unlink -Rf $DIR &&
+	flux kvs put $DIR.a= &&
+	flux kvs dir $DIR


should this test (and the ls one below) also make sure the directory is output?

Yep, will fix that.

chu11 · 2017-10-26T00:30:03Z

@garlick awesome. A bunch of the tests can probably be moved from t1002-kvs-extra.t to the main t1000-kvs.t now that they are more "core" tests.

chu11 · 2017-10-26T00:46:28Z

just finished skimming. Nothing stands out to me as a problem.

Before your rebase, it felt like there were a lack of basic --raw tests. There are some now due to copy-fromkvs and copy-tokvs being gone, but some basic --raw functionality tests would probably be good.

A good test I thought of was to mix treeobj & raw. Off the top of my head:

flux kvs put --raw $DIR.a={ a tree object val }
flux kvs get --raw $DIR.a (get the treeobj above)
flux kvs get --treeobj $DIR.a (can get a treeobj)
flux kvs get $DIR.a (can get value in the treeobj)

chu11 · 2017-10-26T02:21:18Z

t/t1000-kvs.t

+	flux kvs put $DIR.a.b=goodbye &&
+	flux kvs put --treeobj $DIR.b=$(flux kvs get --treeobj $DIR.a) &&
+	(flux kvs get $DIR.a.a && flux kvs get $DIR.a.b) >snap.expected &&
+	(flux kvs get $DIR.b.a && flux kvs get $DIR.b.b) >snap.actual


chain-lint needs &&?

Good catch!

garlick · 2017-10-26T03:59:03Z

Just pushed some changes to address your comments, except I didn't add any new raw tests or move any tests between the two sharness scripts.

I agree the test scripts are a little haphazard in the way they've accreted tests over time and could use a cleanup pass (possibly cleanup is for another time though?)

I'll go through the raw tests including the converted ones tomorrow and see if there are any obvious gaps.

garlick · 2017-10-26T04:50:10Z

restarted one builder after hang in the cron tests

chu11 · 2017-10-26T04:52:33Z

agreed, we should cleanup the tests another day. After this PR is done, we can make an issue for it. I'm sure after append support, the tests haphazardness will just be weirder. We may want to split it up amongst even more files instead of just "core" and "extra".

garlick · 2017-10-26T04:56:04Z

I didn't entirely understand what was meant here - maybe I am misreading

flux kvs put --raw $DIR.a={ a tree object val }
flux kvs get --raw $DIR.a (get the treeobj above)
flux kvs get --treeobj $DIR.a (can get a treeobj)
flux kvs get $DIR.a (can get value in the treeobj)

the first command would encode { tree object val } inside a tree object val, the second would pull it out again. Is nesting those meaningful?

On reorganizing - a good start would be to separate the kvs_watch tests out to their own script since they are a bit complicated and are likely to change when watch is reworked.

coveralls · 2017-10-26T05:01:12Z

Coverage increased (+0.03%) to 78.521% when pulling 2bef84f on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

chu11 · 2017-10-26T15:28:26Z

@garlick ahh, no nesting intended. I guess I shouldn't have used brackets. First line is to just write a treeobj, second line to read it back and make sure it wrote correctly. I also realize now that the 4th line probably should have been with --json. Ugh I really messed up that suggestion.

coveralls · 2017-10-26T15:40:53Z

Coverage increased (+0.03%) to 78.524% when pulling 18474b7 on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

garlick · 2017-10-26T16:15:20Z

I've pushed some raw tests, mainly exercising the kvs put --raw command line.

To be clear, --treeobj gives direct access to the RFC 11 object and --raw creates a 'val' object just like --json or no options. I'm still confused by the suggestion - maybe we should chat face to face.

coveralls · 2017-10-26T16:33:14Z

Coverage increased (+0.07%) to 78.56% when pulling 908824a on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

Problem: flux-kvs put presumes values should be stored as JSON, but the KVS no longer requires this. Add type options to allow the user to choose how values are stored. If no options, value is stored as a NULL terminated string. If --raw, value is stored as with no options, but without a NULL terminator. For --raw mode only, key=- may be used to take read value from stdin. If --json, value is stored as a NULL terminated string if it is valid encoded JSON; otherwise it is first encoded as a JSON string. This mimics the old default behavior of flux-kvs put that is expected by many tests in t1000-kvs.t and t1002-kvs-extra.t. Add --json to flux kvs put where used in various sharness tests. Partial fix to flux-framework#1159.

Problem: flux-get put presumes values should be interpreted as JSON, but the KVS no longer requires this. Add type options mirroring those added to the "put" subcommand to allow the user to choose how values are interpreted. If no options, value is interpreted as a NULL terminated string and written to stdout with a newline. If --raw, value is interpreted as raw data and is written to stdout without any formatting. If --json, value is interpreted as encoded JSON. If the value is an object or array, or any scalar type but string, it is displayed in its encoded form. If the value is a JSON string, quotes are removed, which mimics the old default behavior of flux-kvs get expected by many sharness tests. Add --json to flux kvs get where used in various sharness tests. Fixes flux-framework#1159

garlick · 2017-10-26T20:15:13Z

Squashed.

coveralls · 2017-10-26T20:40:42Z

Coverage increased (+0.02%) to 78.51% when pulling 35700cb on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

chu11 · 2017-10-26T20:54:53Z

src/cmd/flux-kvs.c

+    char *val, *kv, *p;
+    bool overflow = false;
+
+    assert (maxcol == 0 || maxcol > 3);


just realized, I don't think there's a check for a reasonable maxcol in callers of this function. Should a check be added somewhere? Or perhaps there should just be a generic "can't output with such small width" kind of error message?

Yes you're right.

chu11 · 2017-10-26T21:02:38Z

doc/man1/flux-kvs.adoc


-*put* 'key=value' ['key=value...']::
+*put* [-j|-r] 'key=value' ['key=value...']::


need -t and -n

I could swear I already did that one. Thanks!

chu11 · 2017-10-26T21:02:40Z

doc/man1/flux-kvs.adoc

+If '-j', it is first encoded as JSON, then stored as a NULL-terminated string.
+If '-r', it is stored as raw data with no termination.  For raw mode only,
+a value of "-" indicates that the value should be read from standard input.
+If '-t', value is stored as a RFC 11 object.


and description of -n.

Right! Adding that.

chu11 · 2017-10-26T21:20:20Z

other than those nits above, I think things look good. Did you want to squash?

garlick · 2017-10-26T21:34:29Z

Thanks! Re-squashed.

Problem: the KVS allows non-JSON values to be stored, but flux kvs dir -R exits with an error if it encounters one. For each value, if it decodes as JSON, format it like before. But if it doesn't, print it directly instead of exiting. If the output is a terminal, truncate long values so they fit within the terminal width (reusing logic from the ls subcommand). Also truncate values where they contain unprintable characters or newlines. Indicate truncated values by appending "...". Fixes flux-framework#1158

If --treeobj is specified, display RFC 11 object directly instead of parsing it. Allow directories to be listed too.

If --treeobj is specified store value argument as a RFC 11 object. Example, assign snapshot of 'lwj' to 'lwj.snapshot': flux kvs put --treeobj \ lwj.snapshot=$(flux kvs get --treeobj lwj)

Problem: flux kvs get (no options) segfaults if key refers to a zero-length value. If value is zero-length, print nothing instead of handing a NULL pointer to printf.

Problem: if value is zero-length, get --json displays "null", but it should be an error like any other value that doesn't decode as valid JSON, for example the empty string. output_key_json_string() is doing double duty with the watch subcommand, which still treats a NULL value as a non-existent key. Don't pass a NULL value to that function; instead, treat it as an error beforehand.

Add flux-kvs get/readlink/dir --at REF option that allows a lookup to start with a snapshot reference.

Add put --no-merge flag which ensures that flux_commit() is called with the FLUX_KVS_NO_MERGE flag.

Ensure that flux kvs get --treeobj and flux kvs put --treeobj work as designed.

Ensure that an empty string value (just a NULL) can be created and parsed where appropriate.

Replace "t/kvs/basic get-treeobj" with "flux kvs get --treeobj" and "t/kvs/basic put-treeobj" with "flux kvs put --treeobj" in sharness test. Drop get-treeobj, put-treeobj subcommands from test program.

Replace "t/kvs/basic dirsize" with "flux kvs ls -1 | wc -l" and drop the dirsize subcommand from the test program.

Replace "t/kvs/basic getat" with "flux kvs get --at", "t/kvs/basic readlinkat" with "flux kvs readlink --at", and "t/kvs/basic dirat" with "flux kvs dir --at". Drop getat, readlinkat, dirat subcommands from the test program.

Replace "t/kvs/basic put-no-merge" with "flux kvs put --no-merge --json" and drop the put-no-merge subcommand from the test program.

Replace "t/kvs/basic copy-fromkvs" with "flux kvs get --raw" and "t/kvs/basic copy-tokvs key -" with "flux kvs put --raw key=-", and drop copy-fromkvs/copy-tokvs subcommands from test program.

Drop this test kvs: zero size raw value can be stored and retrieved It is basically the same as this one in t1000-kvs.t: kvs: zero-length value handled by put/get --raw

Describe get/put [--json|--raw|--treeobj] options. Describe get/dir/readlink [--at treeobj] option. Describe dir truncation to fit output window and [-w COL] option.

coveralls · 2017-10-26T21:55:33Z

Coverage increased (+0.04%) to 78.528% when pulling 14b3581 on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

coveralls · 2017-10-26T22:17:30Z

Coverage increased (+0.05%) to 78.539% when pulling 14b3581 on garlick:kvs_cmd_update into b12d027 on flux-framework:master.

chu11 · 2017-10-26T22:18:46Z

everything, lgtm!

Problem: The "module-load: no jobs are lost" subtest of sharness t1004-module-load.t is failing. The test is comparing "status" (with quotes) to status (without quotes). The value in the KVS is a JSON string, but flux-framework/flux-core#1257 changed flux-kvs so that it displays raw values by default, in this case the encoded JSON string with the quotes. Add -j to the flux kvs get command in the test, to get the old behavior.

chu11 reviewed Oct 25, 2017

View reviewed changes

garlick force-pushed the kvs_cmd_update branch from 8135869 to 99a1795 Compare October 26, 2017 00:09

chu11 reviewed Oct 26, 2017

View reviewed changes

garlick added 2 commits October 26, 2017 13:06

garlick force-pushed the kvs_cmd_update branch from 908824a to 35700cb Compare October 26, 2017 20:14

chu11 reviewed Oct 26, 2017

View reviewed changes

garlick mentioned this pull request Oct 26, 2017

kvs: sharness test scripts need reorganization #1259

Closed

garlick force-pushed the kvs_cmd_update branch from ee42344 to 14b3581 Compare October 26, 2017 21:33

garlick added 18 commits October 26, 2017 14:36

cmd/flux-kvs: add get --treeobj option

9bafdf3

If --treeobj is specified, display RFC 11 object directly instead of parsing it. Allow directories to be listed too.

cmd/flux-kvs: add put --treeobj option

1a97f58

If --treeobj is specified store value argument as a RFC 11 object. Example, assign snapshot of 'lwj' to 'lwj.snapshot': flux kvs put --treeobj \ lwj.snapshot=$(flux kvs get --treeobj lwj)

cmd/flux-kvs: get handles zero-length value

9a3023b

Problem: flux kvs get (no options) segfaults if key refers to a zero-length value. If value is zero-length, print nothing instead of handing a NULL pointer to printf.

cmd/flux-kvs: add get/readlink/dir [--at REF] option

bc90e5f

Add flux-kvs get/readlink/dir --at REF option that allows a lookup to start with a snapshot reference.

cmd/flux-kvs: add put --no-merge flag

d6910ab

Add put --no-merge flag which ensures that flux_commit() is called with the FLUX_KVS_NO_MERGE flag.

t/t1000-kvs.t: add zero-length value tests

f98b2be

t/t1000-kvs.t: add treeobj tests

c7728fc

Ensure that flux kvs get --treeobj and flux kvs put --treeobj work as designed.

t/t1000-kvs.t: add empty string value tests

2a90ed0

Ensure that an empty string value (just a NULL) can be created and parsed where appropriate.

t/t1000-kvs.t: add raw value tests

ed1874c

t/kvs/basic: drop get-treeobj, put-treeobj subcommands

02df823

Replace "t/kvs/basic get-treeobj" with "flux kvs get --treeobj" and "t/kvs/basic put-treeobj" with "flux kvs put --treeobj" in sharness test. Drop get-treeobj, put-treeobj subcommands from test program.

t/kvs/basic: drop dirsize subcommand

26a8d5a

Replace "t/kvs/basic dirsize" with "flux kvs ls -1 | wc -l" and drop the dirsize subcommand from the test program.

t/kvs/basic: drop getat/readlinkat/dirat subcommands

e012377

Replace "t/kvs/basic getat" with "flux kvs get --at", "t/kvs/basic readlinkat" with "flux kvs readlink --at", and "t/kvs/basic dirat" with "flux kvs dir --at". Drop getat, readlinkat, dirat subcommands from the test program.

t/kvs/basic: drop put-no-merge subcommand

c70d54c

Replace "t/kvs/basic put-no-merge" with "flux kvs put --no-merge --json" and drop the put-no-merge subcommand from the test program.

t/kvs/basic: drop copy-fromkvs/copy-tokvs subcommands

d29640b

Replace "t/kvs/basic copy-fromkvs" with "flux kvs get --raw" and "t/kvs/basic copy-tokvs key -" with "flux kvs put --raw key=-", and drop copy-fromkvs/copy-tokvs subcommands from test program.

t/t1002-kvs-extra.t: drop redundant test

3a7f031

Drop this test kvs: zero size raw value can be stored and retrieved It is basically the same as this one in t1000-kvs.t: kvs: zero-length value handled by put/get --raw

doc/flux-kvs(1): update for new options

14b3581

Describe get/put [--json|--raw|--treeobj] options. Describe get/dir/readlink [--at treeobj] option. Describe dir truncation to fit output window and [-w COL] option.

chu11 merged commit aeb79c3 into flux-framework:master Oct 26, 2017

garlick deleted the kvs_cmd_update branch October 27, 2017 21:42

grondo mentioned this pull request May 10, 2018

0.9.0 Release #1479

Closed

		@@ -56,6 +56,16 @@ static void dump_kvs_dir (const flux_kvsdir_t *dir, bool Ropt, bool dopt);


		put 'key=value' ['key=value...']::
		put [-j\|-r] 'key=value' ['key=value...']::

kvs: handle non-JSON values in flux kvs get, put, dir #1257

kvs: handle non-JSON values in flux kvs get, put, dir #1257

Conversation

garlick commented Oct 25, 2017

coveralls commented Oct 25, 2017

chu11 commented Oct 25, 2017

garlick commented Oct 25, 2017

Choose a reason for hiding this comment

chu11 Oct 25, 2017 • edited

Choose a reason for hiding this comment

codecov-io commented Oct 26, 2017 • edited

Codecov Report

garlick commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 commented Oct 26, 2017

chu11 commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garlick commented Oct 26, 2017 • edited

garlick commented Oct 26, 2017

chu11 commented Oct 26, 2017

garlick commented Oct 26, 2017

coveralls commented Oct 26, 2017

chu11 commented Oct 26, 2017

coveralls commented Oct 26, 2017

garlick commented Oct 26, 2017

coveralls commented Oct 26, 2017

garlick commented Oct 26, 2017

coveralls commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chu11 commented Oct 26, 2017

garlick commented Oct 26, 2017

coveralls commented Oct 26, 2017

coveralls commented Oct 26, 2017

chu11 commented Oct 26, 2017

chu11 Oct 25, 2017 •

edited

codecov-io commented Oct 26, 2017 •

edited

garlick commented Oct 26, 2017 •

edited